Implement env and required struct tag support#12
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThe mflags package adds environment-variable and required-field support. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mflags.go (1)
1821-1823: DocumentenvandrequiredinFromStruct's public tag list.These tags are now part of the exported surface, but the
FromStructdoc comment still omits them, so generated docs and IDE help are already stale.Also applies to: 1886-1887
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mflags.go` around lines 1821 - 1823, Update the public doc comment for the FromStruct function to include the newly supported "env" and "required" struct tag keys in its documented tag list (reference: FromStruct), and do the same for the second FromStruct doc block near the other occurrence; explicitly list "env" (for environment-variable name) and "required" (boolean "true"/"false") so generated docs and IDE tooltips reflect the current exported surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mflags.go`:
- Around line 1886-1897: The code currently sets defaultValue and hasEnvValue
when an env var exists, but must instead apply the env string through the flag
implementation and only mark the flag as having a value on successful parsing;
modify the logic around envVar/defaultValue/hasEnvValue so that after
registering the flag (the Flag with its Value implementation) you call
Flag.Value.Set(envVal) (or otherwise use the flag's parse path) and only set
HasValue = true if Set returns nil; for pointer-backed flags, repeatable slices
(e.g., []bool/[]int) and other unsupported kinds, either apply via
Flag.Value.Set or reject the env tag with an error, and make the same fix in the
other occurrence referenced (the block around the second env handling at the
2077-2082 region) so env-backed flags don’t get marked satisfied when parsing
fails.
---
Nitpick comments:
In `@mflags.go`:
- Around line 1821-1823: Update the public doc comment for the FromStruct
function to include the newly supported "env" and "required" struct tag keys in
its documented tag list (reference: FromStruct), and do the same for the second
FromStruct doc block near the other occurrence; explicitly list "env" (for
environment-variable name) and "required" (boolean "true"/"false") so generated
docs and IDE tooltips reflect the current exported surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfa15ec6-6d92-46b5-9d88-40a20bfacff8
📒 Files selected for processing (3)
help_doc.gomflags.gomflags_test.go
The runtime CLI has been using env:"VAR_NAME" and required:"true" struct tags for a while now, but mflags never actually honored them. They were silently ignored. With strict tag validation coming in MIR-985, upgrading mflags in the runtime repo would surface these as errors, so we need to teach mflags what they mean first. env tags populate a flag's default from an environment variable, giving us layered precedence: default < env < CLI arg. required tags validate after parsing that the flag or positional was given a value (via CLI or env). Both tags work on flags and positionals. Help output now shows (env: VAR_NAME) annotations, and the JSON help doc includes envVar and required fields. A new ErrRequired sentinel lets callers distinguish required-validation failures from other parse errors.
The env var path for positionals was ignoring the error from setFieldValue, which would silently swallow parse failures (e.g. "abc" for an int positional). Now we propagate the error.
The previous approach routed env values through the defaultValue string, which meant parse errors were silently swallowed and pointer/slice types never received the value. Now env values are applied via flag.Value.Set() after registration, which validates the value and works uniformly across all types. Also documents env and required tags in FromStruct's doc comment.
After rebasing on MIR-985's strict tag validation, env and required need to be added to knownTags so FromStruct doesn't reject them. Updates the test that was asserting they were unknown.
phinze
force-pushed
the
phinze/mir-986-add-env-and-required-struct-tag-support-to-mflags
branch
from
087ef1f to
cad95e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mflags.go (1)
49-52:setFlagsmap is populated but never read.The
setFlagsmap is initialized at line 879 and populated during flag parsing (lines 1541, 1567, 1583, 1592), but it is never read or accessed anywhere in the codebase. If this field is intended for future use, consider adding a clarifying comment. Otherwise, it may be removed as dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mflags.go` around lines 49 - 52, The setFlags map field on the mflags struct is written during Parse (assignments like setFlags[...] = true) but never read; either remove the dead field and all assignments that initialize or populate setFlags, or keep it and add a clarifying comment on the setFlags field (and consider adding an accessor if external use is intended) so future readers know it’s intentionally retained; locate the symbol setFlags on the mflags struct and the assignments in Parse to update accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mflags.go`:
- Around line 49-52: The setFlags map field on the mflags struct is written
during Parse (assignments like setFlags[...] = true) but never read; either
remove the dead field and all assignments that initialize or populate setFlags,
or keep it and add a clarifying comment on the setFlags field (and consider
adding an accessor if external use is intended) so future readers know it’s
intentionally retained; locate the symbol setFlags on the mflags struct and the
assignments in Parse to update accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b9e35ad-9c76-4e5b-b2a0-56cfd54ecda9
📒 Files selected for processing (2)
mflags.gomflags_test.go
Was populated during Parse but never read. The HasValue bool on Flag handles required validation directly, making this redundant.
phinze
deleted the
phinze/mir-986-add-env-and-required-struct-tag-support-to-mflags
branch
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mflags.go (1)
1671-1676:⚠️ Potential issue | 🟠 MajorFail fast when a tagged field never registers.
With
envandrequirednow treated as recognized tags, an unsupported field likefloat64 \long:"count" env:"COUNT" required:"true"`` returns nil, registers nothing, ignores the env binding, and skips required validation entirely. Please return an error when a field has flag tags but its kind falls through the registration switch.Also applies to: 1981-2178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mflags.go` around lines 1671 - 1676, The code currently adds "env" and "required" to knownTags but silently ignores fields whose kind falls through the registration switch (so tagged fields like float64 `long:"count" env:"COUNT" required:"true"` neither register nor validate); update the field-registration logic (the function that iterates struct fields and registers flags — e.g., the registerField / registration switch around lines 1981-2178) to detect when any of the tags in knownTags are present on a field but no registration branch ran, and return an explicit error indicating the unhandled kind and tags; implement this by checking parsed tags (from the existing tag parsing path) and, after the switch/default path, if no registration occurred and any known tag was set, return an error describing the field name, type/kind, and the tags so callers fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mflags.go`:
- Around line 49-50: validateRequired currently reads cached slices
requiredFlags and requiredPos (populated only by FromStruct) instead of the live
booleans on Flag and PositionalField, so callers mutating Lookup(...).Required
or GetPositionalFields() are ignored; change validateRequired to consult
Flag.Required and PositionalField.Required directly rather than
requiredFlags/requiredPos, remove or stop using those cached slices where
validation is expected (and update places at the other mentioned regions that
perform required checks to use the .Required booleans), and ensure FromStruct
still sets the .Required fields so both struct-based and programmatic uses are
honored.
- Around line 23-31: The PositionalField struct exposes parser-only fields
Required and HasValue which breaks backwards compatibility for unkeyed struct
literals; change those fields to unexported names (required, hasValue) and keep
them used only internally in the parser (e.g., inside GetPositionalFields and
other parsing logic), and if external callers need their values expose them via
accessors or the existing documentation types (add methods or populate
PositionalDoc/FlagDoc) rather than exported struct fields; update any references
to PositionalField.Required and .HasValue within the package to the new
unexported names and provide public getters or doc generation use where
necessary.
---
Outside diff comments:
In `@mflags.go`:
- Around line 1671-1676: The code currently adds "env" and "required" to
knownTags but silently ignores fields whose kind falls through the registration
switch (so tagged fields like float64 `long:"count" env:"COUNT" required:"true"`
neither register nor validate); update the field-registration logic (the
function that iterates struct fields and registers flags — e.g., the
registerField / registration switch around lines 1981-2178) to detect when any
of the tags in knownTags are present on a field but no registration branch ran,
and return an explicit error indicating the unhandled kind and tags; implement
this by checking parsed tags (from the existing tag parsing path) and, after the
switch/default path, if no registration occurred and any known tag was set,
return an error describing the field name, type/kind, and the tags so callers
fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb01d1d0-97a7-40f7-97bc-cfc3becb0d07
📒 Files selected for processing (3)
help_doc.gomflags.gomflags_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- help_doc.go
- mflags_test.go
|
Hey coderabbit, you know the review is supposed to come before the merge, right? We're going to have to get you a watch. 🐰⌚ On the remaining findings: the silent fallthrough for unsupported kinds with |
2 participants
The runtime CLI has been using
env:"VAR_NAME"andrequired:"true"struct tags on dozens of config fields, but mflags never actually honored either of them. They've been silently ignored this whole time. With MIR-985 adding strict tag validation, upgrading mflags in the runtime repo would surface all of these as errors, so we need to teach mflags what they mean before that upgrade lands.The
envtag populates a flag's default from an environment variable duringFromStruct, giving us clean layered precedence: hard-coded default < env var < CLI argument. No changes toParsewere needed since env just acts as a better default that gets overwritten if the user passes the flag explicitly.The
requiredtag validates after parsing that the flag or positional was actually given a value, either via CLI arg or env var. AHasValuebool on each flag tracks whether it was populated by any source, andvalidateRequired()runs at the end ofParseto check them all. Missing required flags produce anErrRequiredsentinel so callers can distinguish validation failures from other parse errors.Both tags work on flags and positionals. Help output now shows
(env: VAR_NAME)annotations, and the JSON help doc includesenvVarandrequiredfields for tooling consumers.Closes MIR-986